fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

jcdhlzq · 2023-05-05T12:42:40Z

Summary: Fix text-cutoff phenomenon in non-Fabric mode on Android device, like #15114, #29259 and many other similar issues that not fixed, and onTextLayout with wrong TextLayout params.

The text-cutoff phenomenon is fixed in native side with a proper layout procedure rather than making a work-around in JavaScript side.

The TextLayout params for onTextLayout callback will be fixed to offer reasonable x and y for each line, especially for RTL languages like Arabic. This problem is shown below.

Changelog:

[ANDROID] [FIXED] - Fix Text cutoff problem in non-Fabric mode.
[ANDROID] [FIXED] - Fix TextLayout params of onTextLayout callback.

Test Plan:

It has been tested on many devices to make sure that the problems are fixed and Text component can work better than that of old version.

The devices are: Pixel 5 Android 13, Xiaomi 10 Pro Android 10, Xiaomi 12 Android 12, Vivo X9L Android 7.1.2, Vivo Y85A Android 8.1, Oppo Reno8 Android13, Oppo A3 Andrroid 10, Oppo R9s Android 6.0.1, Oppo R5m Android 5.1 and Huawei Nova2s Harmony 2.0.

Next two cases on Pixel 5 and Xiaomi 10 Pro wil be shown for proving the effect.

Xiaomi 10 Pro

You could find that text-cutoff problem is well fixed from the screeshots of Xiaomi 10 Pro. And the fixed version works better than the old version on Pixel 5, because the fixed version offers better onTextLayout callback. The evidence will be presented in the next section.

Old version

There are many texts cut off.

Fixed version

No text cutoff phenomenon and all text are better than the old version.

Pixel 5

Now cases for onTextLayout callback will be presented in this section.

Old version

The x and y of all the lines is not in correct place.

Fixed version

Work better!

facebook-github-bot · 2023-05-05T12:42:46Z

Hi @jcdhlzq!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

analysis-bot · 2023-05-05T13:07:53Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,733,482	+202
android	hermes	armeabi-v7a	8,044,661	+200
android	hermes	x86	9,223,589	+205
android	hermes	x86_64	9,075,558	+185
android	jsc	arm64-v8a	9,298,443	+365
android	jsc	armeabi-v7a	8,487,179	+363
android	jsc	x86	9,359,616	+373
android	jsc	x86_64	9,615,702	+366

Base commit: 6d24ee1
Branch: main

facebook-github-bot · 2023-05-06T03:15:49Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

gnprice · 2023-05-09T18:02:38Z

Interesting, thanks for sending this.

Does this also fix #25481? That's the issue that #15114 was closed as a duplicate of.

jcdhlzq · 2023-05-10T05:41:12Z

Interesting, thanks for sending this.

Does this also fix #25481? That's the issue that #15114 was closed as a duplicate of.

Yes, I just tested this issue on the OnePlus device with Roboto font family. I reproduced it and it proved to be fixed with this pr.

The issue #25481 can be reproduced when changing system default font family to Roboto, like the following picture.

But the same operation don't reproduce the problem.

…nes prop is less than Layout.getLineCount()

jcdhlzq · 2023-05-16T02:47:43Z

Anybody knows what's going on with this pr?

jcdhlzq · 2023-05-23T12:26:09Z

Hi, @gnprice ! Do you know what is happening with this PR？

jcdhlzq · 2023-06-02T06:52:49Z

What's wrong with this PR? @facebook-github-bot

jcdhlzq · 2023-06-02T12:21:08Z

Do you know what is happening about this PR? @cipolleschi

cipolleschi · 2023-06-05T11:04:20Z

Hi, what do you mean with "what is happening about this PR?" Are you seeing something unusual or is it just taking longer than usual to get reviewed?

facebook-github-bot · 2023-06-05T11:06:25Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

jcdhlzq · 2023-06-05T11:10:32Z

Hi, what do you mean with "what is happening about this PR?" Are you seeing something unusual or is it just taking longer than usual to get reviewed?

It simplely means that there is no any information or any state change. So I cannot figure out if I should do something.

cipolleschi · 2023-06-05T11:13:46Z

@jcdhlzq Ok, thanks. I'll try to push it forward!

javache

We'd need these changes as well in the new architecture, ReactTextShadowNode is only used in the old architecture.

javache · 2023-06-07T11:16:17Z

...act-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java

+          TextView textView =
+            Assertions.assertNotNull(mInternalView, "mInternalView cannot be null");
+
+          return measureWithView(text, textView, width, widthMode, height, heightMode);


Yoga measures on a background thread. It's unsafe to access TextView here.

I'm sorry, could you offer some points exactly about the unsafe access? If it means some cases like #17530 , I think they are different. Because EditText has a background by default while TextView doesn't.

<style name="Widget.EditText"> <item name="focusable">true</item> <item name="focusableInTouchMode">true</item> <item name="clickable">true</item> <item name="background">?attr/editTextBackground</item> <item name="textAppearance">?attr/textAppearanceMediumInverse</item> <item name="textColor">?attr/editTextColor</item> <item name="gravity">center_vertical</item> <item name="breakStrategy">simple</item> <item name="hyphenationFrequency">@dimen/config_preferredHyphenationFrequency</item> <item name="defaultFocusHighlightEnabled">false</item> </style>

<style name="Widget.TextView"> <item name="textAppearance">?attr/textAppearanceSmall</item> <item name="textSelectHandleLeft">?attr/textSelectHandleLeft</item> <item name="textSelectHandleRight">?attr/textSelectHandleRight</item> <item name="textSelectHandle">?attr/textSelectHandle</item> <item name="textEditPasteWindowLayout">?attr/textEditPasteWindowLayout</item> <item name="textEditNoPasteWindowLayout">?attr/textEditNoPasteWindowLayout</item> <item name="textEditSidePasteWindowLayout">?attr/textEditSidePasteWindowLayout</item> <item name="textEditSideNoPasteWindowLayout">?attr/textEditSideNoPasteWindowLayout</item> <item name="textEditSuggestionItemLayout">?attr/textEditSuggestionItemLayout</item> <item name="textEditSuggestionContainerLayout">?attr/textEditSuggestionContainerLayout</item> <item name="textEditSuggestionHighlightStyle">?attr/textEditSuggestionHighlightStyle</item> <item name="textCursorDrawable">?attr/textCursorDrawable</item> <item name="breakStrategy">high_quality</item> <item name="hyphenationFrequency">@dimen/config_preferredHyphenationFrequency</item> </style>

Moreover, There are many cases where YogaMeasureFunction measures in the same way like ReactSwitch, ProgressBar.

I think the change is that with this PR, the ShadowNode now manages a new TextView on the background thread that is only used during layout?

So this is moving from laying out Spannable to laying out a background text element.

This is how TextInput works today on Paper, but Fabric, for both Text and TextInput, still measures off of the spannable/AttributedString.

I think the change is that with this PR, the ShadowNode now manages a new TextView on the background thread that is only used during layout?

So this is moving from laying out Spannable to laying out a background text element.

This is how TextInput works today on Paper, but Fabric, for both Text and TextInput, still measures off of the spannable/AttributedString.

That's right!

I'm seeing if we can dig up the context on why Fabric chose to avoid this style of measurement, but this is a pretty major change. Usually we'd want to A/B test something like this, but that isn't really a possibility because most of our internal apps do not run Paper-specific code. This also means we receive very limited test coverage, since this code is not exercised by tests against most of our own apps.

If it is possible to find the state we are missing in the layout, that would likely be more palatable. Do you know if this issue reproduces in Fabric?

@NickGerleman I'm also pretty concerned about memory here, as it means we now have two TextViews for every Text element on screen.

I've had A/B test on various devices with different ROMs and put it on PRT environment and No any problem or bad case feedback reported.
As for Fabric mode, we don't use that architecture yet.

@NickGerleman I'm also pretty concerned about memory here, as it means we now have two TextViews for every Text element on screen.

Later, maybe after a few days, I'll have a test about peak memory difference and explain the root cause for this phenomenon which is related to Paint.nGetRunAdvance, a native method which may be modified by different ROM vendors. For recent days, I am too busy to take care about this PR in time. Sorry about that!

@NickGerleman I'm also pretty concerned about memory here, as it means we now have two TextViews for every Text element on screen.

The memory diff is about 1.2kb when measuring with TextView.

I'm seeing if we can dig up the context on why Fabric chose to avoid this style of measurement, but this is a pretty major change. Usually we'd want to A/B test something like this, but that isn't really a possibility because most of our internal apps do not run Paper-specific code. This also means we receive very limited test coverage, since this code is not exercised by tests against most of our own apps.

If it is possible to find the state we are missing in the layout, that would likely be more palatable. Do you know if this issue reproduces in Fabric?

@NickGerleman Yes, it reproduces when running on the app built with newArchEnabled=true.

NickGerleman · 2023-06-09T03:47:42Z

@mdvacca got back to me offline with the history that we moved away from background component based measurement because of both performance and thread safety challenges.

So, I don’t think we’re going to be able to accept this solution.

jcdhlzq · 2023-07-04T04:00:19Z

@mdvacca got back to me offline with the history that we moved away from background component based measurement because of both performance and thread safety challenges.

So, I don’t think we’re going to be able to accept this solution.

Sorry, I don't get what the history is and could you offer some information about it ?

As for the performance and thread safety challeges you referred, there is no evidence in our prt environment over the past months.(For critical reasons, we have put it on prt env to make users have correct texts like $98, otherwise, users may have texts like $9)

github-actions · 2023-12-31T05:01:20Z

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

github-actions · 2024-01-07T05:13:35Z

This PR was closed because it has been stalled for 7 days with no activity.

KingAmo · 2024-08-16T02:11:37Z

could you please provide a work-around ?

fix: text cut off phenomenon on some Android devices like Xiaomi

875c04e

jcdhlzq mentioned this pull request May 5, 2023

Text Cut Off on Android 13 #36992

Closed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 6, 2023

fix: Fix NoSuchMethodError on Android api level below M

f5444b7

jcdhlzq mentioned this pull request May 10, 2023

Width measures of Text components using a custom font are wrong #25481

Open

lizhiqiang05 added 2 commits May 11, 2023 19:34

fix: Text component may get wrong height when the value of numberOfLi…

b924760

…nes prop is less than Layout.getLineCount()

fix: add setFallbackLineSpacing call

72eb600

jcdhlzq added 2 commits May 19, 2023 11:30

Merge branch 'facebook:main' into fix/text-cut

9123480

Merge branch 'facebook:main' into fix/text-cut

aabea1b

jcdhlzq closed this Jun 2, 2023

jcdhlzq reopened this Jun 5, 2023

javache requested changes Jun 7, 2023

View reviewed changes

github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 31, 2023

github-actions bot closed this Jan 7, 2024

fabOnReact mentioned this pull request Jan 26, 2024

[Feature] - PRs about Text and TextInput fabOnReact/react-native-improved#10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

jcdhlzq commented May 5, 2023 •

edited

Loading

facebook-github-bot commented May 5, 2023

analysis-bot commented May 5, 2023 •

edited

Loading

facebook-github-bot commented May 6, 2023

gnprice commented May 9, 2023

jcdhlzq commented May 10, 2023 •

edited

Loading

jcdhlzq commented May 16, 2023

jcdhlzq commented May 23, 2023

jcdhlzq commented Jun 2, 2023

jcdhlzq commented Jun 2, 2023

cipolleschi commented Jun 5, 2023

facebook-github-bot commented Jun 5, 2023

jcdhlzq commented Jun 5, 2023 •

edited

Loading

cipolleschi commented Jun 5, 2023

javache left a comment

javache Jun 7, 2023

jcdhlzq Jun 8, 2023

NickGerleman Jun 8, 2023 •

edited

Loading

jcdhlzq Jun 8, 2023

NickGerleman Jun 8, 2023 •

edited

Loading

javache Jun 8, 2023

jcdhlzq Jun 8, 2023

jcdhlzq Jun 12, 2023

jcdhlzq Jul 3, 2023

jcdhlzq Jul 3, 2023 •

edited

Loading

NickGerleman commented Jun 9, 2023

jcdhlzq commented Jul 4, 2023

github-actions bot commented Dec 31, 2023

github-actions bot commented Jan 7, 2024

KingAmo commented Aug 16, 2024

fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

fix: text cut off phenomenon on some Android devices like OPPO, Xiaomi and etc #37271

Conversation

jcdhlzq commented May 5, 2023 • edited Loading

Summary: Fix text-cutoff phenomenon in non-Fabric mode on Android device, like #15114, #29259 and many other similar issues that not fixed, and onTextLayout with wrong TextLayout params.

Changelog:

Test Plan:

Xiaomi 10 Pro

Pixel 5

facebook-github-bot commented May 5, 2023

Action Required

Process

analysis-bot commented May 5, 2023 • edited Loading

facebook-github-bot commented May 6, 2023

gnprice commented May 9, 2023

jcdhlzq commented May 10, 2023 • edited Loading

jcdhlzq commented May 16, 2023

jcdhlzq commented May 23, 2023

jcdhlzq commented Jun 2, 2023

jcdhlzq commented Jun 2, 2023

cipolleschi commented Jun 5, 2023

facebook-github-bot commented Jun 5, 2023

jcdhlzq commented Jun 5, 2023 • edited Loading

cipolleschi commented Jun 5, 2023

javache left a comment

Choose a reason for hiding this comment

javache Jun 7, 2023

Choose a reason for hiding this comment

jcdhlzq Jun 8, 2023

Choose a reason for hiding this comment

NickGerleman Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

jcdhlzq Jun 8, 2023

Choose a reason for hiding this comment

NickGerleman Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

javache Jun 8, 2023

Choose a reason for hiding this comment

jcdhlzq Jun 8, 2023

Choose a reason for hiding this comment

jcdhlzq Jun 12, 2023

Choose a reason for hiding this comment

jcdhlzq Jul 3, 2023

Choose a reason for hiding this comment

jcdhlzq Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

NickGerleman commented Jun 9, 2023

jcdhlzq commented Jul 4, 2023

github-actions bot commented Dec 31, 2023

github-actions bot commented Jan 7, 2024

KingAmo commented Aug 16, 2024

jcdhlzq commented May 5, 2023 •

edited

Loading

analysis-bot commented May 5, 2023 •

edited

Loading

jcdhlzq commented May 10, 2023 •

edited

Loading

jcdhlzq commented Jun 5, 2023 •

edited

Loading

NickGerleman Jun 8, 2023 •

edited

Loading

NickGerleman Jun 8, 2023 •

edited

Loading

jcdhlzq Jul 3, 2023 •

edited

Loading